Fix create-song navigation race#39
Open
johannesschiessl wants to merge 2 commits into
Open
Conversation
johannesschiessl
marked this pull request as ready for review
July 24, 2026 17:12
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Architecture diagram
sequenceDiagram
participant UI as Song Detail Route
participant CreatedSongHandoff as CreatedSongHandoff Module
participant SongAtoms as SongAtoms Layer
participant ShowLayout as ShowLayout Component
participant SongService as Backend SongService
participant Repository as Show Repository
Note over UI,Repository: Creating a Song
UI->>UI: makeClientId(songIdPrefix)
Note over UI: Generate stable client-side ID<br/>using crypto.getRandomValues
UI->>SongAtoms: Optimistic create with client ID
SongAtoms->>SongAtoms: Use client ID instead of temporary ID
UI->>SongService: songs.create({ showId, id, name, artist, ... })
SongService->>Repository: update document
alt Song ID already exists (retry scenario)
Repository->>SongService: Existing document
SongService->>SongService: Return existing song (no duplicate)
else New song
Repository->>SongService: Updated document with new song
SongService->>SongService: Return song from updated document
end
SongService-->>UI: Created song response
UI->>CreatedSongHandoff: remember(showId, song, insertAfterSongId)
Note over UI,CreatedSongHandoff: Bridge until stream catches up
UI->>UI: navigate to /shows/$showId/setlist/$songId
alt Navigation succeeds
UI-->>UI: Navigate to detail page
else Navigation fails
UI->>UI: Show error "The song was added,<br/>but its page could not be opened"
end
Note over UI,Repository: Detail Route Rendering
ShowLayout->>CreatedSongHandoff: reconcile(showId, streamedSongs)
Note over ShowLayout: Called when songsResult becomes success
CreatedSongHandoff->>CreatedSongHandoff: Remove non-pending songs from handoff
UI->>ShowLayout: Subscribe to songAtoms
ShowLayout-->>UI: songsResult (success/failure/initial)
alt Initial state and no handoff song
UI->>UI: Show loading empty state
else Failure state, empty list, and no handoff song
UI->>UI: Show error empty state
else Normal rendering
UI->>CreatedSongHandoff: find(showId, songId)
Note over UI: Fallback to handoff if not in streamed list
UI->>CreatedSongHandoff: provisionalNumber(showId, songId, syncedSongs)
Note over UI: Calculate position relative to anchor song
alt Song deleted
UI->>CreatedSongHandoff: forget(showId, songId)
UI->>UI: Navigate back to setlist
end
end
Note over UI,Repository: Stream Failure Handling
UI->>ShowLayout: songsResult with previousSuccess
ShowLayout->>ShowLayout: Use last successful snapshot
Note over UI,ShowLayout: Preserve last known setlist during<br/>transient stream failures
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Confidence score: 5/5
- In
apps/web/src/components/songs/CreatedSongHandoff.ts,reconcileis using rawsyncedSongswhile pending optimistic items live only in the separatesongsatom, so the production handoff path can drop in-flight optimistic snapshots and cause users to see a brief missing/reverted song state during navigation — pass pending optimistic entries intoreconcile(or merge atoms before handoff) so the guard can preserve them.
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/src/components/songs/CreatedSongHandoff.ts">
<violation number="1" location="apps/web/src/components/songs/CreatedSongHandoff.ts:42">
P3: Pending optimistic snapshots are not retained here: `reconcile` receives raw `syncedSongs`, while `pending` exists only on the separate optimistic `songs` atom. This guard never returns in production, so route handoff can still be discarded on the next differing raw snapshot; pass optimistic pending state into reconciliation (or remove this ineffective guard and its misleading test).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| songs: ReadonlyArray<Song & { readonly pending?: boolean }>, | ||
| snapshot: object, | ||
| ) => { | ||
| if (songs.some((song) => song.pending)) return; |
Contributor
There was a problem hiding this comment.
P3: Pending optimistic snapshots are not retained here: reconcile receives raw syncedSongs, while pending exists only on the separate optimistic songs atom. This guard never returns in production, so route handoff can still be discarded on the next differing raw snapshot; pass optimistic pending state into reconciliation (or remove this ineffective guard and its misleading test).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/songs/CreatedSongHandoff.ts, line 42:
<comment>Pending optimistic snapshots are not retained here: `reconcile` receives raw `syncedSongs`, while `pending` exists only on the separate optimistic `songs` atom. This guard never returns in production, so route handoff can still be discarded on the next differing raw snapshot; pass optimistic pending state into reconciliation (or remove this ineffective guard and its misleading test).</comment>
<file context>
@@ -25,9 +37,18 @@ export const makeCreatedSongHandoff = () => {
) => {
- for (const song of songs) {
- if (!song.pending) forget(showId, song.id);
+ if (songs.some((song) => song.pending)) return;
+
+ const syncedIds = new Set(songs.map((song) => song.id));
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Root cause
Song persistence and the streaming setlist refresh are serialized, so the create RPC can return before the next full list snapshot reaches the device. Navigation immediately opened the returned song ID, while the detail route treated the older snapshot as conclusive and rendered “Song not found.” A response lost after persistence could also make a retried create ambiguous and potentially duplicate the song.
User impact
A successfully created song opens reliably even on slower or reconnecting devices. Actual create failures remain on the current page with their RPC error, while a separate navigation failure explicitly says the song was added.
Verification
SongService: 6 tests passedCreatedSongHandoff: 3 tests passedThe repository-wide sweep also exercised the new tests successfully. Its unrelated mDNS advertiser suite failed in the local dependency patch/mock path, and the desktop typecheck reported the same missing patched
fixedNamedeclaration; neither area is changed by this PR.